Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Orchestrator implementation #53

Merged
merged 32 commits into from
May 5, 2020
Merged

Orchestrator implementation #53

merged 32 commits into from
May 5, 2020

Conversation

jessicayuen
Copy link
Member

@jessicayuen jessicayuen commented Apr 8, 2020 •
edited
Loading

Add the end-to-end request management workflow.
See comments inline for details.

Three open issues to follow up on (also inline in code):
#71
#70
#68

Signed-off-by: Jess Yuen jyuen@lyft.com

@jessicayuen
Return Resource on cache Fetch
124d83e 
* Return `Resource` rather than `Response` on fetch. Orchestrator needs
  the Resource object afterall in order to identify and notify watches.
* Export `Resource` and `Response` members that will be used by calling
  classes.

Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Orchestrator implementation
8c07603 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
jyotimahapatra
jyotimahapatra reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor

@jyotimahapatra jyotimahapatra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first pass looks great. This is a complex piece. Some questions.

internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/upstream/client.go Outdated Show resolved Hide resolved
@jessicayuen jessicayuen linked an issue Apr 9, 2020 that may be closed by this pull request
Add orchestrator interface #43
Closed
@jessicayuen
Pass by reference for cache watch, export MarshalResources
8fbde42 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Add downstream, upstream response map wrappers
ee54b65 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Changes and fixes to orchestrator workflow
3b505db 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Add unit tests
071e8e5 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen jessicayuen marked this pull request as ready for review April 13, 2020 06:18
@jessicayuen
Add missing testdata
0e853a8 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Link TODO with issues
6234b8a 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Fanout response to watches in parallel
fad0c13 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Send response only if req/resp versions differ
7723d2c 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
jyotimahapatra
jyotimahapatra reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@jyotimahapatra jyotimahapatra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more questions.

internal/app/orchestrator/downstream.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
@jessicayuen
Remove sleep
78ecf04 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Merge master
2f15c34 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Add TODOs, feedback
97c91e7 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Use sync.Map
aeef7ca 
To support per-key locking.

Signed-off-by: Jess Yuen <jyuen@lyft.com>
jyotimahapatra
jyotimahapatra reviewed Apr 29, 2020
View reviewed changes
internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
@jessicayuen
s/shutdown/shutdownUpstream
5315967 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
add timeout
d3d1e3b 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
add wg
6bcaf46 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
jessicayuen added 3 commits May 1, 2020 14:08
@jessicayuen
Reduce timeout, don't close downstream channels
30246e7 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
fanout with default, buffered downstream chans
ecb0a28 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Remove timeout
144d156 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen jessicayuen requested a review from lita May 4, 2020 20:44
@jessicayuen
Copy link
Member Author

@jyotimahapatra Addressed all comments, can you take another pass?

lita
lita reviewed May 5, 2020
View reviewed changes
internal/app/orchestrator/upstream.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator.go Show resolved Hide resolved
internal/app/cache/cache.go Show resolved Hide resolved
internal/app/cache/cache.go Show resolved Hide resolved
internal/app/orchestrator/downstream.go Show resolved Hide resolved
internal/app/orchestrator/orchestrator_test.go Show resolved Hide resolved
lita
lita reviewed May 5, 2020
View reviewed changes
internal/app/orchestrator/orchestrator.go
select {
case channel <- convertToGcpResponse(resp, *watch):
break
default:
Copy link

@lita lita May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often is this expected to happen? It looks like this channel is buffered as 1. if there is spam of updates, maybe it is fine to drop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's more discussion for this here.

The idea behind this was that a downstream receiver could be blocked for any reason. Initially I had went with a timeout, but in discussions with @jyotimahapatra we realized that it would imply the slowest sidecar would block new updates from being sent to other sidecars, due to the use of WaitGroups. We can't get rid of WaitGroups because more recent updates might get overriden by a slower update if updates were bursty.

The buffer is necessary in order to give time for the receiver to initialize. I realized in this test that the second watch (for the same aggregated key) would fall to the default case if the channel was unbuffered.

If there is a spam of updates, I think it's reasonable to drop since only one of those updates will be applied downstream anyway, but we do lose the the guarantee that it is the "latest" update. Could move back to the configured timeout model if this is a concern, but I put some of those downsides ^.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts are to move forward with the default for now. I'll document this. If it is a problem in practice, then we can revisit the timeout model or other approaches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me.

Copy link

@lita lita May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah in the future, you can do what Envoymanager does and drop the oldest and enqueue the newest rather than logging an error.

That has been working quite well for us since none of the configurations requires ordering and we send the state of the world. We always just want the latest update.

@lita
Copy link

lita commented May 5, 2020 •
edited
Loading

Overall, this looks great. Most of my comments are around documentation of the code, but the logic of the implementation makes sense to me. I think it is fair to press forward.

lita
lita previously approved these changes May 5, 2020
View reviewed changes
@jessicayuen
Document upstream/downstream files
a36fc60 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jyotimahapatra
Copy link
Contributor

@jessicayuen The changes look great. I was thinking there's scope for adding some negative tests too for the orchestrator. If you want to add those in a separate PR, i would be ok with that.

jessicayuen added 2 commits May 5, 2020 12:45
@jessicayuen
Heavily document watchUpstreams, s/watches/watchers
d08ddcc 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
assert sync map keys
4e4e0cc 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
comment on default
57ff78e 
Signed-off-by: Jess Yuen <jyuen@lyft.com>
@jessicayuen
Copy link
Member Author

Sounds reasonable @jyotimahapatra, let me add that in a separate PR or as a part of the e2e tests - this PR is already getting a bit unwieldy and I would like to get a base point in.

@jessicayuen jessicayuen merged commit c5ea288 into envoyproxy:master May 5, 2020
@gnz00
Copy link

gnz00 commented May 8, 2020

@lita I might be misunderstanding, there are some ordering concerns with ADS. There has been a few issues about it on the go-control-plane repo (one of them from myself).

@kyessenov summarizes it best, union updates and version transactions: envoyproxy/go-control-plane#218 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnz00 gnz00 gnz00 left review comments

@jyotimahapatra jyotimahapatra jyotimahapatra approved these changes

@lita lita lita approved these changes

@eapolinario eapolinario Awaiting requested review from eapolinario

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add orchestrator interface
4 participants
@jessicayuen @lita @jyotimahapatra @gnz00